fix(codex): ignore user config in subprocesses#488
Conversation
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
8127f13for PR #488
Review record:
baad0e3f-6896-40a8-97ef-b7c1ecf4dee6
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | src/ouroboros/orchestrator/codex_cli_runtime.py:555 | BLOCKING | Forcing codex exec --ignore-user-config breaks Ouroboros’ own Codex integration contract. setup.py explicitly registers the Ouroboros MCP/env hookup in ~/.codex/config.toml; skipping user config means runtime-launched Codex sessions will no longer load that registration, so tool-backed flows that depend on the configured MCP server stop working in production even though setup succeeded. There is no compensating path in this diff and no test covering execution with the configured Codex MCP hookup enabled. |
| 2 | src/ouroboros/providers/codex_cli_adapter.py:361 | BLOCKING | The adapter now also suppresses ~/.codex/config.toml unconditionally. That removes any Codex-side MCP/tool configuration the caller relies on, and for Ouroboros specifically it bypasses the setup-managed MCP/env hookup the project tells users to install there. Since this adapter allows tool-using completions (allowed_tools defaults to unrestricted), this is a behavior regression rather than a harmless isolation change, and the added tests only assert the flag is present instead of covering the configured-tool path that now breaks. |
Non-blocking Suggestions
None.
Design Notes
The change is mechanically small but crosses a real contract boundary: setup/uninstall treat ~/.codex/config.toml as the authoritative Codex integration point, while these command builders now opt out of reading it entirely. If isolation is needed, it should be selective rather than disabling the project’s own configured Codex environment.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
|
I don't think we should merge this in its current form. The isolation goal is understandable, but forcing That makes this too broad. If there is a specific inheritance problem we want to solve here, please narrow it first:
Then solve that targeted problem directly instead of globally opting out of the configured Codex environment. At minimum, any version of this change needs coverage showing that a setup-managed Codex MCP/env hookup still works end-to-end after the change. Right now the regression risk is larger than the isolation benefit. |
|
I agree that this is a bit of a blunt instrument. The intent was to be able to have different settings for codex invocations that are run by ouroboros vs those I've initiated interactively from the desktop or cli. For instance, when I'm working on small tasks interactively without ouroboros, I want the best model, fast mode, and I want particular notify actions (sounds), that I don't want when ouroboros is using codex. I also don't necessarily want certain mcp configs enabled for ouroboros workers. Digging deeper, I think the best way to do this might be to use codex profiles, which are still in the config.toml file, and then modify ouroboros to take a codex_profile config setting that gets passed to |
|
The profile selection approach might also be a good way to implement the reasoning tier levels needed for #184 |
Q00
left a comment
There was a problem hiding this comment.
I understand the intent here: Codex subprocesses should be isolated enough that a parent/user session does not accidentally leak behavior into Agent OS runs. That is a real problem to solve.
I do not think --ignore-user-config is the right default lever, though. In Ouroboros, setup-managed Codex integration lives in the user config path as well, including MCP/env wiring. If we force codex exec --ignore-user-config, we can also disable the very configuration that makes Codex work correctly inside Ouroboros.
For Agent OS, the safer contract is: default runs should preserve the setup-managed Codex configuration, and isolation should be explicit/narrow. A profile-based design sounds much better to me, e.g. an optional codex_profile config that passes codex exec --profile ... when set. That would let us avoid parent-session drift without globally discarding user config.
Requesting changes because this version is too broad and can break the default Codex/MCP path even though CI is green. I would be happy with a replacement PR that keeps the default config path intact and adds coverage showing setup-managed MCP/env config still works.
|
I think the Codex profile idea is the right direction, but I would probably frame it as the Codex mapping of a more general Agent OS The contract I have in mind is: That keeps the default Codex config path intact, avoids the broad behavior of A good first slice could be:
So yes, I like the profile direction. I would just prefer to land it under the Agent OS runtime-profile abstraction, with Codex profile as the first backend mapping, instead of making |
|
@andrew-adamson — thanks for sticking with this through the redesign discussion. The Agent OS `runtime_profile` slice you and @Q00 sketched is now up at #505, following the four-step scope from his comment above:
Your interactive vs worker use case is exactly what the worker profile is for — drop your per-worker overrides (model, notify, sandbox) into that section and they apply to every Ouroboros-spawned `codex exec` without touching your interactive defaults. The profile abstraction also gives us a clean home for #184 and the next backends (OpenCode/Hermes/Claude/LiteLLM) as separate slices. @Q00 — happy to leave the close on this PR to you once #505 lands; or if you'd rather close now, fine by me. |
|
Yep, #505 sounds like the way to go, so I'm happy to close this. |
Adds an opt-in `orchestrator.runtime_profile` setting that lets every runtime/adapter map a single abstract profile name to its native configuration mechanism. Phase 1 ships only the Codex mapping (`worker` → `codex exec --profile ouroboros-worker`); OpenCode, Hermes, Claude Code, and LiteLLM mappings are intentionally deferred to follow-up slices. This is the cooperative, multi-runtime alternative to PR Q00#488's blunt `--ignore-user-config`: setup-managed `[mcp_servers.ouroboros]` and the new `[profiles.ouroboros-worker]` section co-exist in `~/.codex/config.toml`, so isolation is selective rather than wholesale, and worker subprocesses no longer have to disable Ouroboros's own MCP/env hookup to escape interactive Codex defaults. Default `runtime_profile=None` preserves today's `_build_command` output exactly — covered by regression tests in both runtime and adapter — so existing installations see zero behavior change until they opt in. Closes the design discussion on Q00#488. Refs Agent OS RFC Q00#476, Q00#184. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds an opt-in `orchestrator.runtime_profile` setting that lets every runtime/adapter map a single abstract profile name to its native configuration mechanism. Phase 1 ships only the Codex mapping (`worker` → `codex exec --profile ouroboros-worker`); OpenCode, Hermes, Claude Code, and LiteLLM mappings are intentionally deferred to follow-up slices. This is the cooperative, multi-runtime alternative to PR Q00#488's blunt `--ignore-user-config`: setup-managed `[mcp_servers.ouroboros]` and the new `[profiles.ouroboros-worker]` section co-exist in `~/.codex/config.toml`, so isolation is selective rather than wholesale, and worker subprocesses no longer have to disable Ouroboros's own MCP/env hookup to escape interactive Codex defaults. Default `runtime_profile=None` preserves today's `_build_command` output exactly — covered by regression tests in both runtime and adapter — so existing installations see zero behavior change until they opt in. Closes the design discussion on Q00#488. Refs Agent OS RFC Q00#476, Q00#184. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
--ignore-user-configto Codex subprocess invocations so runtime behavior does not inherit local user config unexpectedlyTesting
uv run pytest tests/unit/orchestrator/test_codex_cli_runtime.py tests/unit/providers/test_codex_cli_adapter.py